multipart/form-data support for typespec-todo #2984
Conversation
timovv
force-pushed
the
multipart-todo
branch
2 times, most recently
from
c2e63bd to
8c0bd25
Compare
| @@ -646,6 +646,9 @@ export interface FileContentResponse { | |||
| content: Uint8Array; | |||
| } | |||
|
|
|||
| // @public | |||
| export type FileContents = string | NodeJS.ReadableStream | ReadableStream<Uint8Array> | Uint8Array | Blob; | |||
There was a problem hiding this comment.
just curious why this is added? is this expected?
There was a problem hiding this comment.
Seems like this spec has an anonymous model for MFD which isn't properly supported yet. FileContents should be used in uploadFile but is currently Uint8Array. Will fix.
There was a problem hiding this comment.
Will address anonymous models including this case in a follow up issue
packages/typespec-ts/test/modularUnit/scenarios/multipart/file.md
Outdated
Show resolved
Hide resolved
packages/typespec-ts/test/modularUnit/scenarios/multipart/file.md
Outdated
Show resolved
Hide resolved
packages/typespec-ts/test/modularUnit/scenarios/multipart/file.md
Outdated
Show resolved
Hide resolved
|
|
||
| ```tsp | ||
| model RequestBody { | ||
| files: HttpPart<File>[]; |
There was a problem hiding this comment.
should we add a case for
model RequestBody {
files: HttpPart<File[]>;
}
There was a problem hiding this comment.
I don't think that case really makes sense. That body model would represent a single part consisting of a JSON array of File objects.
We do already have a case for a JSON array here.
| @@ -128,6 +130,9 @@ function emitType(context: SdkContext, type: SdkType, sourceFile: SourceFile) { | |||
| if (isAzureCoreErrorType(context.program, type.__raw)) { | |||
| return; | |||
| } | |||
| if (isOrExtendsHttpFile(context.program, type.__raw!)) { | |||
There was a problem hiding this comment.
I wonder if we could use usage to filter the HttpPart models out?
There was a problem hiding this comment.
We can't because tcgc doesn't set UsageFlags.MultipartFormData for Http.File. It only does that for the actual multipart body model.
| ); | ||
| if ( | ||
| !properties.some((x) => x.name === name) && | ||
| !(existInterface && existInterface.getProperty(name)) |
There was a problem hiding this comment.
I wonder why we need this filter? do you see any cases where there's duplication?
There was a problem hiding this comment.
Yes, the multipart payload cadl ranch spec causes this duplication for some reason. If you revert this change and regenerate you should see the issue.
| NameType.Property | ||
| ); | ||
|
|
||
| // HACK: check if the statement includes a group of this name already to prevent an operation group appearing multiple times |
There was a problem hiding this comment.
I feel like it's strange if an operation group would appear multiple times. how could I repro?
There was a problem hiding this comment.
Same issue as above -- without this the multipart spec generates the group 3 times. I think it might have to do with the way that the operations are nested?
packages/typespec-test/test/todo_non_branded/generated/openapi/openapi.json
Show resolved
Hide resolved
timovv
changed the title
packages/typespec-ts/test/modularUnit/scenarios/example/example.md
Outdated
Show resolved
Hide resolved
| parts.push(partDefinition); | ||
| } | ||
|
|
||
| // TODO: How to handle additionalProperties for MFD? |
There was a problem hiding this comment.
is there an issue to track this?
There was a problem hiding this comment.
Added it to the follow up issue #3001
multipart/form-datasupport for the cases covered by the todo spec for the e2e demo.Includes basic support for file uploads, JSON, and text parts. There are some items to follow up on after this PR:
@multipartBodyare not currently supportedFixes #2422